Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block library: Refactor color supports handling for mobile #22502

Merged
merged 1 commit into from
May 26, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 20, 2020

Description

Addresses the following comment:

#21326 (comment)

I stumbled upon this PR while trying to move supports to block.json:
#22422 (comment)

It looks like colors are handled differently on mobile. What's the reason for using conditions like:

__experimentalColor: Platform.OS === 'web',

Would it be okay if we would not run the code that uses this flag on mobile instead? It would be still valuable to share the same configuration between mobile and web. You need to keep in mind that this is something that site owners or plugin authors can modify to customize their experience.

This PR moves the check to the file that uses __experimentalColor support flag and ensures this feature is disabled for the mobile app. It was the remaining blocker to move supports to block.json file that will allow exposing those settings in the new REST API endpoint that's in works.

How has this been tested?

There should be no difference in how both web and mobile operating.

Currently, on mobile, we are supporting colors only in a few blocks such as Buttons or Cover.

Those blocks should be validated on mobile.

Types of changes

Refactoring.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@gziolo gziolo requested a review from lukewalczak May 20, 2020 16:16
@gziolo gziolo added [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality labels May 20, 2020
@gziolo gziolo mentioned this pull request May 20, 2020
6 tasks
@gziolo gziolo self-assigned this May 20, 2020
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label May 20, 2020
@github-actions
Copy link

Size Change: -25 B (0%)

Total Size: 1.11 MB

Filename Size Change
build/block-editor/index.js 105 kB +16 B (0%)
build/block-library/index.js 119 kB -41 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 180 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.24 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.6 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.8 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.73 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@gziolo gziolo added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) and removed [Status] In Progress Tracking issues with work in progress labels May 21, 2020
@gziolo gziolo requested review from geriux and dratwas May 21, 2020 07:21
@gziolo gziolo marked this pull request as ready for review May 21, 2020 07:22
@dratwas
Copy link
Contributor

dratwas commented May 21, 2020

Hey, @gziolo These changes are valid in the current state but I'm afraid that we will support the color flag in mobile blocks one by one. What should we do if we would like to support the color flag for a paragraph at first and then add it to other blocks? Should we create our own list with blocks that support that flag? We could also turn off the color flag for all blocks as it is implemented in this PR and add it manually. I mean add color edit to the InspectorControls as in the Button block case. https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/button/edit.native.js#L522

Our plan was to add support of showing colors in blocks and then just remove the check Platform.OS === 'web' to automatically add the ColorEdit in the InspectorControls.

Nothing changes from our perspective since we support colors only for Button which doesn't use the color flag at all. But later, we would like to use that flag and we would need to support it for all blocks (with the __experimentalColor) or we couldn't use it at all.

@gziolo
Copy link
Member Author

gziolo commented May 21, 2020

These changes are valid in the current state but I'm afraid that we will support the color flag in mobile blocks one by one. What should we do if we would like to support the color flag for a paragraph at first and then add it to other blocks? Should we create our own list with blocks that support that flag?

Thanks for checking, can you keep a list of blocks as an additional check for mobile? Something like:

const hasColorSupport = ( blockType ) =>
 	( Platform.OS === 'web' || [ 'core/block-1', 'core/block-2' ].includes( blockType ) )
        && hasBlockSupport( blockType, COLOR_SUPPORT_KEY );

@dratwas
Copy link
Contributor

dratwas commented May 22, 2020

Thanks for checking, can you keep a list of blocks as an additional check for mobile? Something like:

It is what i meant

Should we create our own list with blocks that support that flag?

I think we can, however, IMO it should be a temporary solution and ultimately we would like to support the color flag on mobile as well :)

cc: @lukewalczak @pinarol

@pinarol
Copy link
Contributor

pinarol commented May 22, 2020

@dratwas thanks for confirming, could you also give a quick test to this PR on mobile side and approve accordingly? Let's also check columns block inserter support as it was also leaning on a support flag.

Copy link
Contributor

@dratwas dratwas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked it on mobile and it seems like everything works as before.

Let's also check columns block inserter support as it was also leaning on a support flag.

Columns block works fine as well.

@gziolo gziolo merged commit a917162 into master May 26, 2020
@gziolo gziolo deleted the update/color-supports-mobile branch May 26, 2020 07:13
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone May 26, 2020
@gziolo
Copy link
Member Author

gziolo commented May 26, 2020

Thank you @dratwas for cooperation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants